loc: replace reflection-based args with tuple-params + IDictionary overloads#354
Conversation
Localization_LocaleSwitching now runs under AOT (PR #354), so: 193 skipped -> 192 skipped ~542 passed -> ~543 passed Total fixture count stays at 735. Also drops "anonymous-type localization args" from the documented skip-cluster list since that subsystem is now AOT-clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
AOT verification complete. Full AOT selftest run against the published binary on this branch:
The 40 newly-passing sub-assertions cover Greeting / Plural_Zero / Plural_Five / Search_Zero / Search_One / Search_Many / Direction across Pushed |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR removes reflection-based localization argument handling (which breaks under NativeAOT trimming) and replaces it with AOT-safe argument passing via IDictionary<string, object> and tuple-params overloads for Message/RichMessage.
Changes:
- Updated
IntlAccessorto accept explicit dictionaries and added tuple-params overloads that build args without reflection. - Migrated call sites (tests, fixtures, CLI source rewriter) from anonymous objects to tuple args / dictionaries.
- Updated documentation and AOT skip list to reflect the new, AOT-safe localization args model.
Show a summary per file
| File | Description |
|---|---|
| tests/Reactor.Tests/Localization/SourceRewriterTests.cs | Updates assertions to match tuple-arg codegen output. |
| tests/Reactor.Tests/Localization/RichMessageTests.cs | Switches rich-message args to explicit dictionary. |
| tests/Reactor.Tests/Localization/IntlAccessorTests.cs | Migrates tests from anonymous args objects to tuple args. |
| tests/Reactor.AppTests.Host/SelfTest/SelfTestRunner.cs | Removes localization fixture from AOT skip list and updates rationale. |
| tests/Reactor.AppTests.Host/SelfTest/Fixtures/LocalizationFixtures.cs | Migrates localization fixture calls to tuple args. |
| tests/Reactor.AppTests.Host/Fixtures/LocalizationFixtures.cs | Migrates app-test fixture calls to tuple args. |
| src/Reactor/Core/Localization/IntlAccessor.cs | Core API change: dictionary-based args + tuple overloads; removes reflection helpers. |
| src/Reactor.Cli/Loc/SourceRewriter.cs | Emits tuple-arg syntax instead of anonymous objects. |
| docs/reference/localization-howto.md | Updates example to use tuple args. |
| docs/aot-support.md | Removes anonymous-type localization args as an AOT limitation. |
| docs/_pipeline/templates/localization.md.dt | Updates narrative to describe tuple args as the primary interpolation shape. |
| docs/_pipeline/apps/localization/App.cs | Updates code snippets to use tuple args. |
| CONTRIBUTING.md | Updates expected AOT skip/pass counts after removing the localization skip. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 5
Five inline findings from copilot-pull-request-reviewer:
1. BuildArgs null-skip semantics. Behavior parity with the removed
ToArgsDictionary, which dropped null-valued properties so the formatter
saw the placeholder as missing rather than substituting "null". Tuple
value type is now object?, nulls are skipped before insertion, and
a unit test pins the behavior.
2. cref XML doc warnings. The RichMessage <see cref=...> referenced a
method signature with C# keyword generics and nullable annotations,
which can fail to resolve. Replaced with a prose reference and a
<c>RichMessage</c> code element.
3. SourceRewriter param-name escaping (x2). The generator emitted
$"(\"{param}\", ...)" without escaping the placeholder name. ICU spec
doesn't forbid " or \ in param names — if one slipped through,
the emitted source became invalid C#. Now uses
SymbolDisplay.FormatLiteral to produce a valid C# string literal.
4. Docs template phrasing. (name, value) reads like a positional
tuple with an identifier first item; readers might try
.Message(key, (name, value)). Tightened to ("name", value) and
added a sentence explaining the first item is the placeholder name
as a string literal.
Also: ran mur docs compile --skip-screenshots, which regenerated
docs/guide/localization.md (template change) and docs/guide/navigation.md
(picked up an unrelated drift from the navigation doc app that an
earlier commit forgot to regen). Both checked in so docs/guide stays
authoritative.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed all 5 reviewer findings in 615b10f:
JIT localization tests: 283/283 (the +1 is the new null-skip case). |
Localization_LocaleSwitching now runs under AOT (PR #354), so: 193 skipped -> 192 skipped ~542 passed -> ~543 passed Total fixture count stays at 735. Also drops "anonymous-type localization args" from the documented skip-cluster list since that subsystem is now AOT-clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Five inline findings from copilot-pull-request-reviewer:
1. BuildArgs null-skip semantics. Behavior parity with the removed
ToArgsDictionary, which dropped null-valued properties so the formatter
saw the placeholder as missing rather than substituting "null". Tuple
value type is now object?, nulls are skipped before insertion, and
a unit test pins the behavior.
2. cref XML doc warnings. The RichMessage <see cref=...> referenced a
method signature with C# keyword generics and nullable annotations,
which can fail to resolve. Replaced with a prose reference and a
<c>RichMessage</c> code element.
3. SourceRewriter param-name escaping (x2). The generator emitted
$"(\"{param}\", ...)" without escaping the placeholder name. ICU spec
doesn't forbid " or \ in param names — if one slipped through,
the emitted source became invalid C#. Now uses
SymbolDisplay.FormatLiteral to produce a valid C# string literal.
4. Docs template phrasing. (name, value) reads like a positional
tuple with an identifier first item; readers might try
.Message(key, (name, value)). Tightened to ("name", value) and
added a sentence explaining the first item is the placeholder name
as a string literal.
Also: ran mur docs compile --skip-screenshots, which regenerated
docs/guide/localization.md (template change) and docs/guide/navigation.md
(picked up an unrelated drift from the navigation doc app that an
earlier commit forgot to regen). Both checked in so docs/guide stays
authoritative.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
615b10f to
3cb6a07
Compare
…erloads
The Message(MessageKey, object?) and RichMessage(MessageKey, object?, ...)
overloads built an args dictionary by reflecting over public properties of
the passed object — anonymous types or [LocArgs]-marked records. Under
NativeAOT the trimmer keeps the ctor + backing fields of the anonymous type
(both reachable from the
ew { … } call site) but drops the property
getters, because nothing statically reads them. GetProperties() then
returned an empty array, MessageFormat threw on {name}, the catch
swallowed the exception, and the raw ICU pattern leaked through to the UI.
This change removes the reflection path entirely and gives callers two
AOT-safe shapes:
- Message(MessageKey, IDictionary<string, object>?) — explicit dict for
cases that already have one (or want to share/build one programmatically).
- Message(MessageKey, (string Name, object Value) arg1,
params (string Name, object Value)[] more) — compact tuple
syntax for the common inline case. ValueTuple is a nominal type with
concrete fields; building a Dictionary<string,object> from tuple
values needs zero reflection.
Same shape for RichMessage. (RichMessage with tags uses the dict
overload — params must come last, so the tuple variant can't carry
the optional ags argument.)
Deleted:
- ToArgsDictionary + IsAcceptableArgsContainer + _propertyCache
- The IL2026/IL2070 unconditional-suppression attributes those needed
- System.Reflection + System.Collections.Concurrent +
System.Diagnostics.CodeAnalysis usings (now unused)
Migrated callers:
- Selftest + AppTests localization fixtures (two files)
- IntlAccessorTests, RichMessageTests
- SourceRewriter CLI codegen: .Message(key, new { name = expr })
→ .Message(key, ("name", expr)). Updated SourceRewriterTests.
- Docs: _pipeline/templates/localization.md.dt narrative and
_pipeline/apps/localization/App.cs snippets, plus the
hand-edited docs/reference/localization-howto.md example.
Removed from AOT skip list / docs:
- Localization_LocaleSwitching no longer needs to be skipped under AOT
( ests/Reactor.AppTests.Host/SelfTest/SelfTestRunner.cs).
- The "Localization with anonymous-type args" row in docs/aot-support.md
is gone — there's no anonymous-type path left.
Verified:
- dotnet build src/Reactor clean (AOT warnings as errors).
- 282 localization unit tests pass.
- Full JIT selftest: 0 failures.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Localization_LocaleSwitching now runs under AOT (PR #354), so: 193 skipped -> 192 skipped ~542 passed -> ~543 passed Total fixture count stays at 735. Also drops "anonymous-type localization args" from the documented skip-cluster list since that subsystem is now AOT-clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Five inline findings from copilot-pull-request-reviewer:
1. BuildArgs null-skip semantics. Behavior parity with the removed
ToArgsDictionary, which dropped null-valued properties so the formatter
saw the placeholder as missing rather than substituting "null". Tuple
value type is now object?, nulls are skipped before insertion, and
a unit test pins the behavior.
2. cref XML doc warnings. The RichMessage <see cref=...> referenced a
method signature with C# keyword generics and nullable annotations,
which can fail to resolve. Replaced with a prose reference and a
<c>RichMessage</c> code element.
3. SourceRewriter param-name escaping (x2). The generator emitted
$"(\"{param}\", ...)" without escaping the placeholder name. ICU spec
doesn't forbid " or \ in param names — if one slipped through,
the emitted source became invalid C#. Now uses
SymbolDisplay.FormatLiteral to produce a valid C# string literal.
4. Docs template phrasing. (name, value) reads like a positional
tuple with an identifier first item; readers might try
.Message(key, (name, value)). Tightened to ("name", value) and
added a sentence explaining the first item is the placeholder name
as a string literal.
Also: ran mur docs compile --skip-screenshots, which regenerated
docs/guide/localization.md (template change) and docs/guide/navigation.md
(picked up an unrelated drift from the navigation doc app that an
earlier commit forgot to regen). Both checked in so docs/guide stays
authoritative.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3cb6a07 to
290eca6
Compare
Why
PR #353 skipped
Localization_LocaleSwitchingunder AOT becauset.Message(key, new { name = "World" })reflects over the anonymous typeto build the ICU args dictionary, and the trimmer drops the property
getters under NativeAOT (ctor + backing fields are statically reachable
from the
new { ... }site, but property getters aren't, since the onlyreader was a reflective
GetProperties()call).GetProperties()returned empty,
MessageFormat.FormatMessagethrew on{name}, thecatchswallowed, and the raw ICU pattern leaked through to the UI.We don't want a reflection-based dispatcher in this code path anyway —
it forces every
[UnconditionalSuppressMessage]we''ve written on thetype, and the security model (refuse arbitrary DTOs via
IsAcceptableArgsContainer) is doing nothing the type system couldn''tdo statically.
What
Replace the
object?-typedargsparameter with two AOT-safe overloads:Callers go from this:
to this:
Same shape for
RichMessage. The tagged variant uses the dict overloadbecause
paramsmust be last:ValueTupleis a nominal type with concrete fields — building aDictionary<string,object>from tuple values needs zero reflection,so the whole IL2026/IL2070 surface for this method disappears.
Deleted
ToArgsDictionary,IsAcceptableArgsContainer,_propertyCache[UnconditionalSuppressMessage("Trimming", "IL2026"/"IL2070")]attrsusing System.Reflection,System.Collections.Concurrent,System.Diagnostics.CodeAnalysis(now unused)[LocArgs]soft opt-in marker (string-name match inIsAcceptableArgsContainer)Migrated callers
IntlAccessorTests,RichMessageTestsReactor.Cli/Loc/SourceRewriter: emits tuple syntax now. CLI codegenfor interpolated strings now writes
t.Message(key, ("name", expr))instead of
t.Message(key, new { name = expr }). UpdatedSourceRewriterTestsassertions._pipeline/templates/localization.md.dtnarrative and_pipeline/apps/localization/App.cssnippets (the compileddocs/guide/localization.mdregenerates from these). Plus thehand-edited
docs/reference/localization-howto.mdexample.AOT skip list
Localization_LocaleSwitchingremoved fromDefaultAotSkipPatternsin
SelfTestRunner.cs.docs/aot-support.md— there''s no anonymous-type path left to break.Verified
dotnet build src/Reactor— clean (AOT warnings remain as errors).dotnet test tests/Reactor.Tests --filter "FullyQualifiedName~Localization"— 282/282 pass.dotnet run --project tests/Reactor.AppTests.Host -- --self-test(JIT, full suite) — 0 failures.
back with numbers.
Breaking change
t.Message(key, new { name = "..." })no longer compiles. The compilererror is reasonably clear ("cannot convert anonymous type to
IDictionary<string, object>"); no analyzer needed. Migration is amechanical
new { name = X }→("name", X).Base
This PR is based on
codemonkeychris/aot-selftest-skips-round3(PR #353). When that merges, GitHub will retarget this PR to
main.